Do not keep task id reference indefinitely in Celery instrumenation #3690
Conversation
rbagd
changed the title
| task_runtime_estimated = (default_timer() - start_time) * 1000 | ||
|
|
||
| metrics = self.get_metrics() | ||
| self.assertEqual(CeleryInstrumentor().task_id_to_start_time, {}) |
There was a problem hiding this comment.
Should another test be added where the task_id does not exist and the pop operation returns None instead?
There was a problem hiding this comment.
Would that test matter actually? We're not testing return value of the pop here in any case. I may not see your point. Can you maybe explain the case you have in mind?
| self.task_id_to_start_time.get(task_id), | ||
| attributes=metric_attributes, | ||
| ) | ||
| self.task_id_to_start_time.pop(task_id, None) |
There was a problem hiding this comment.
What is that we are leaking? On line 355 we are setting this a time object so don't expect to keep alive something on the celery side?
There was a problem hiding this comment.
task_id_to_start_time is a dictionary initialised with CeleryInstrumentor. Each time a task finishes the dictionary is augmented with a record of time spent for that task id. In the current state the dictionary just keeps growing even though the task finished long time ago (task identifiers are normally unique). There are no references to any other Python objects that prevent GC, it's just this relatively small issue.
Co-authored-by: Riccardo Magliocchetti <[email protected]>
3 participants
Description
Reference to a task identifier as a string is kept indefinitely by the instrumentation. This just makes sure it gets removed once no longer needed.
Related #3458
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Does This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.